Toggle group ownership on projects #4792
Conversation
| else | ||
| true | ||
| end | ||
| end |
There was a problem hiding this comment.
Even though this could be condensed by combining the 'None' case and the unset case, I left them separate so that we can replace true in the ternary with the ownership removal later. Otherwise we could simplify it to just
new_group = attributes.fetch(:group_owner, 'None')
(new_group == 'None') ? true : set_group_owner(new_group)There was a problem hiding this comment.
I don't see where this is being called from. Also - None value is likely dangerous if it's ever used. It should default to the users Primary group as that will always be a valid fallback.
There was a problem hiding this comment.
I already removed the 'None' option from the select, as that was only necessary when we were detecting the 'possible groups', which could have been an empty set. Now that we are detecting the user's groups directly, there is no way to submit 'None'. However I see how this could create issues, maybe just sticking with nil as the default is a better idea. It should still cause set_group_owner to rescue, but ideally without any unintended effects
apps/dashboard/app/models/project.rb
Outdated
| @directory = File.expand_path(@directory) unless @directory.blank? | ||
| @template = attributes[:template] | ||
|
|
||
| @group_owner = get_group_owner |
There was a problem hiding this comment.
Although this instance variable is not explicitly used, it is responsible for setting the initial value in the form
|
I don't think I'm a fan of modifying the directory after it's been created. I'd think this is needed in the project creation - but editing or changing it after the fact, I'm not so sure. I don't think changing the group/account/charge-back code after the fact is something that'll ever need to happen. This is because funds ($) are tied to this group/account/charge-back code. A new/different group/account/charge-back code is an entirely new project. Specifying it during project creation? Sure, I think that's needed. |
|
If we specify it during creation, we don't get to do the automagic detection of which groups it is 'shareable' with, since that depends on the location of the project root (unknown before initial creation). Of course this is all an alternative to specifying in the group level directory the proper default group ownership, but that moves the responsibility from the project owner to the center admin.
Isn't that the only way we can toggle projects from private to shared and vice versa? If charge schemes prevent us from modifying group ownership then we could not allow for mistakes to be made during project creation (which seems user-error prone) |
|
I am also trying to account for a universal shared directory where you want to limit your project to a specific group (but multiple groups have access to the parent). Is that something we should be accounting for or not? The use case I had in mind for this was the following interaction when working with non-primary groups This seems like a common enough occurrence that we should find a good solution to it, or putting this responsibility on the admin who creates the |
I don't think there is such a thing as a "universal shared directory" outside of maybe
If we're accounting for a mistake - then maybe - but Changing the ownership after the fact I think just sets us up for a lot of edge case failures. You'd want to
Not sure where you're getting the admin from here. If a user creates |
|
Ok the group selection option has now been enabled for the new page only, and the group is only shown on projects outside the user's home directory (potentially shared projects). We can return to group owner modification later, when we have a way to account for edge cases or have a clearer idea of what that should look like. |
github-project-automation
bot
moved this from Changes Requested
to Merged/Closed
in PR Review Pipeline
github-project-automation
bot
moved this from Merged/Closed
to Awaiting Review
in PR Review Pipeline
apps/dashboard/app/models/project.rb
Outdated
|
|
||
| def make_dir | ||
| project_dataroot.mkpath unless project_dataroot.exist? | ||
| set_group_owner |
There was a problem hiding this comment.
Can we actually move this to update_permissons - I feel like it fits better there.
There was a problem hiding this comment.
Errr.... actually it is fine here. We should maybe just pass 750 to mkpath here.
I say that it's fine here, because we'll also need to setgid bit for shared projects and we should do that before we make the other directories so that they're initialized under the correct group.
There was a problem hiding this comment.
It certainly would logically, but as soon as project_dataroot has files, we end up in the chmod -r scenario that could present a lot of complex cases. By intercepting it as soon as the project directory is created (before it has any contents) we make sure that the group ownership (and eventually setbit) setting can inform the creation of those project files
There was a problem hiding this comment.
We should maybe just pass 750 to mkpath here
I wonder if this prompts a reorganization to the setup steps here. Maybe we have a method make_root that just creates the root directory, and then fire them in the following order: make_root && update_permissions && make_dir.... That way all the permissions changes have the chance to apply before metadata files are created
There was a problem hiding this comment.
I decided to go ahead and make these changes. So the root directory creation has it's own method make_root, chgrp_directory is included in update_permissions, and the flow in save is as I described above. Since we are going to have the setbit stuff coming this sets us up ahead of time.
apps/dashboard/app/models/project.rb
Outdated
| end | ||
|
|
||
| def set_group_owner | ||
| return true if private? || @group_owner == get_group_owner |
There was a problem hiding this comment.
Since it's an att_reader we should prefer to use it instead of referencing the instance variable directly. I.e., group_owner instead of @group_owner.
There was a problem hiding this comment.
Also the name of this method implies it's a setter method, which it doesn't actually set the instance variable. Infact it's chowns the directory, not just retrieve an instance variable.
Can we fix these get_ and set_ signatures? Maybe it's more like default_group_owner and chown_directory respectively.
There was a problem hiding this comment.
Yeah I definitely see how set is a problem with that. For get_group_owner I am less sure, seeing as this is the source of truth that we use to report it later (indeed it returns nil if run on an uninitialized project). 'Default' could also get confused with setgid bit (or other 'default' facl rules) so I would want a different term than that.
For set_group_owner, maybe we want to borrow some other naming patterns from this file and call it update_group_owner (in the pattern of update_permissions)? It cannot set the group to anything other than the group_owner attribute, and I feel like chown_directory may be missing that aspect.
I suppose in both cases there is an issue of ambiguity between the 'actual' group on the project root, and the @group_owner instance variable, and both methods are used to help get these in sync with one another.
There was a problem hiding this comment.
OK, there's also chgrp_directory too.
As to get_group_owner maybe it's just directory_group_owner?
It's just get & set - on the surface - tend to reference getting and setting instance variables so I'd like to avoid them.
There was a problem hiding this comment.
Ok yeah I am happy with those
There was a problem hiding this comment.
I have taken care of renaming the two methods and using the attr_reader instead of getting the variable directly
| <path d="M5.255 5.786a.237.237 0 0 0 .241.247h.825c.138 0 .248-.113.266-.25.09-.656.54-1.134 1.342-1.134.686 0 1.314.343 1.314 1.168 0 .635-.374.927-.965 1.371-.673.489-1.206 1.06-1.168 1.987l.003.217a.25.25 0 0 0 .25.246h.811a.25.25 0 0 0 .25-.25v-.105c0-.718.273-.927 1.01-1.486.609-.463 1.244-.977 1.244-2.056 0-1.511-1.276-2.241-2.673-2.241-1.267 0-2.655.59-2.75 2.286m1.557 5.763c0 .533.425.927 1.01.927.609 0 1.028-.394 1.028-.927 0-.552-.42-.94-1.029-.94-.584 0-1.009.388-1.009.94"/> | ||
| </svg> | ||
| </a> | ||
| HEREDOC |
There was a problem hiding this comment.
I have added the help button to the label by passing html as a HEREDOC string to the bootstrap-form helper. This html includes the question-circle bootstrap icon as an svg. If we do not like the idea of including the svg directly, there are other ways (like adding bootstrap-icons as a dependency), but this seems the most straightforward and stable.
There was a problem hiding this comment.
Yea why not icon in an i tag like this one:
There was a problem hiding this comment.
Yeah that seems good. I figured there was an existing pattern somewhere, but hadn't been able to find it
There was a problem hiding this comment.
This has been done, so should be ready for re-review
johrstrom
left a comment
johrstrom
left a comment
There was a problem hiding this comment.
OK this will work. Though we should now follow up with a little bit of javascript to disable the group choice when the project is private. End users should not be allowed to chgrp on a directory in their HOME, that will just lead to confusion and errors. Their primary group will work just fine in their own HOME.
| end | ||
|
|
||
| def chgrp_directory | ||
| return true if private? || group_owner == directory_group_owner |
There was a problem hiding this comment.
End users should not be allowed to chgrp on a directory in their HOME, that will just lead to confusion and errors.
I already thought of this and covered that case here. While some client-side js might make this more clear to the end user, we at least keep them safe from actually doing it.
There was a problem hiding this comment.
Yea I think we should just reinforce it for the end user - or we'll get questions about why the value they supply does nothing.
github-project-automation
bot
moved this from Awaiting Review
to Merged/Closed
in PR Review Pipeline
Fixes #4780 by adding a form item to edit forms allowing you to set the group owner. This has a few parts to it:
Eventually I would like to show the group owner in the show page, but this is a proof of concept that gets the hard part out of the way. I also think we may want to move the permissions settings to their own modal on the show page (which would more elegantly handle the fact that these can only be set after project creation), but since there is a good deal of frontend work and usage planning that would have to go into that, placing it in the form is a quicker way to get the same functionality.
The one piece this is currently missing is the ability to remove group ownership once it is given. The only way I can think to do this currently is to find a group inside the intersection mentioned above so that only the owner can access, but I don't know if that is guaranteed to exist. That being said, we are eventually going to add options to change facl permissions, so maybe "setting the group owner to 'none'" is actually best implemented by removing group access altogether, though interpreting that correctly (like in
get_group_ownercould get complicated.The last special case here is that you can't use this to target intersections of groups. So if I (member of
group1andgroup2) create a project in a directory only accessible togroup2, I can't change the project owner togroup1knowing that a certain member ofgroup2is also a member ofgroup1(effectively sharing it just with them). In a specialized permissions pane, it would be nice to have a toggle that would ignore the "who can access the parent" filter, but I don't know if this is possible inside a form without getting too complicated.